Skip to content

fix(cuDF): Fix debug build failure#17011

Open
pramodsatya wants to merge 2 commits intofacebookincubator:mainfrom
pramodsatya:fix_cudf_dbg_build
Open

fix(cuDF): Fix debug build failure#17011
pramodsatya wants to merge 2 commits intofacebookincubator:mainfrom
pramodsatya:fix_cudf_dbg_build

Conversation

@pramodsatya
Copy link
Copy Markdown
Collaborator

@pramodsatya pramodsatya commented Apr 2, 2026

Problem

Build error for T = double:

/home/vpcuser/velox/./velox/experimental/cudf/expression/AstUtils.h: In function ‘std::unique_ptr<cudf::scalar> facebook::velox::cudf_velox::makeScalarFromValue(const facebook::velox::TypePtr&, T, bool, std::optional<cudf::type_id>) [with T = double]’:
/home/vpcuser/velox/./velox/experimental/cudf/expression/AstUtils.h:79:41: error: call to ‘cudf::get_default_stream’ declared with attribute error: cudf default stream argument used. Pass stream explicitly.
   79 |   auto stream = cudf::get_default_stream();
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~^~
/home/vpcuser/velox/./velox/experimental/cudf/expression/AstUtils.h:80:8: error: call to ‘cudf::get_current_device_resource_ref’ declared with attribute error: cudf default memory resource argument used. Pass mr explicitly.
   80 |   auto mr = cudf::get_current_device_resource_ref();

makeScalarFromValue() in AstUtils.h calls cudf::get_default_stream() and cudf::get_current_device_resource_ref(). When any .cpp file includes both CudfNoDefaults.h and AstUtils.h (e.g. CudfHashJoin.cpp), these calls hit the __attribute__((error)) redeclarations that guard against accidental default argument usage.

In Release builds (-O2), GCC's dead-code elimination removes unused template instantiations before the error fires. In Debug builds (-O0), all instantiations are preserved, triggering a hard compile error.

Solution

Replaces the two calls with existing approved alternatives:

  • cudf::get_default_stream()cudf::get_default_stream(cudf::allow_default_stream) (from CudfDefaultStreamOverload.h)
  • cudf::get_current_device_resource_ref()rmm::mr::get_current_device_resource() (direct RMM call, same pattern as get_temp_mr() in CudfNoDefaults.h)

Both return identical values at runtime — zero performance impact.

Changes

  • velox/experimental/cudf/expression/AstUtils.h: Update makeScalarFromValue() to use poison-safe alternatives

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 2, 2026
@netlify
Copy link
Copy Markdown

netlify bot commented Apr 2, 2026

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 92d1b4a
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/69d68fd0f0a68b00087d506c

@pramodsatya pramodsatya requested a review from czentgr April 2, 2026 15:21
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

Build Impact Analysis

Full build recommended. Files outside the dependency graph changed:

  • velox/experimental/cudf/expression/AstUtils.h

These directories are not fully covered by the dependency graph. A full build is the safest option.

cmake --build _build/release

Fast path • Graph from main@d7891436cf23f7f76322259344ed7d707b428c60

@pramodsatya pramodsatya requested a review from majetideepak April 3, 2026 15:05
auto stream = cudf::get_default_stream();
auto mr = cudf::get_current_device_resource_ref();
auto stream = cudf::get_default_stream(cudf::allow_default_stream);
auto mr = rmm::mr::get_current_device_resource();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API at velox/experimental/cudf/CudfNoDefaults.h says we could use get_temp_mr() instead.
@bdice can you please take a look at this fix? Thanks.

Copy link
Copy Markdown
Collaborator

@karthikeyann karthikeyann Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We should use get_temp_mr(). Later this get_temp_mr() will be replaced by ops specific temporary mr to track temporary memory allocation too.

Copy link
Copy Markdown
Collaborator

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AST and Evaluator are hard to clean up with temporary mr and stream usage because they are called in non-driver thread in driver adapter while invoking constructor of Operators. so, It was not cleaned at all.

Using get_temp_mr() is right fix for now.

@pramodsatya
Copy link
Copy Markdown
Collaborator Author

Thanks @majetideepak, @karthikeyann, updated as suggested and verified this fixes the cuDF debug build on latest main. Could you please take another look?

@majetideepak majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants